[17.0][IMP] base_tier_validation: Merge module with base_tier_validation_waiting #800

bosd · 2024-01-10T18:52:02Z

As proposed as an idea in:
#787 (comment)
This is the PR for the migration of base_tier_validation_waiting into the base module base_tier_validation_waiting.

Please fasstrack, as v17 is not deployed yet in production, this is the best time to introduce this refactor.

To the contributors/authers/reviewers of the migration pr..

@celm1990, @nguyenminhchien, @bofiltd, @moitabenfdz, @sonhd91, @Dranyel-Bosd, @BT-jcolmeiro
Can you please review?

OCA-git-bot · 2024-01-10T18:52:21Z

Hi @LoisRForgeFlow,
some modules you are maintaining are being modified, check this out!

bosd · 2024-01-10T18:54:56Z

Some background info...

Since sequential validations are supported in the base module. It makes sense from an user perspective to only sent out notifications to the validator if it is thier turn in the sequence to review.
Merging of these two modules accomplish that.

Before this pr (and without the waiting module) it was only possible to sent out and notification when a record was created. A reviewer in the end of the sequence got the e-mail notification before it's turn.
If the user decided to go to the document and make an attempt to do so. He was unable to do it.. Because it was not their turn yet..
Since there was no capability to get a notification. The user- had to visit the record now and then to see if it was their turn to validate.
This was a poor flow from usabulity point of view.
Easily fixed by the waiting module.

The merge of into the base module. Should make it more clear to users/functional consultants. As well as easier to maintain.

bofiltd

Makes sense.
Functional test locally. LGTM 🍾

celm1990

first code review; I will test it for functionality.

celm1990 · 2024-01-11T03:38:34Z

base_tier_validation/models/tier_review.py

+        reviews = self.filtered(lambda rev: rev.status in ["waiting", "pending"])
+        if reviews:
+            # get minimum sequence of all to prevent jumps
+            next_seq = min(reviews.mapped("sequence"))
+            for record in reviews:
+                # if approve by sequence, check sequence has been reached
+                if record.approve_sequence:
+                    if record.sequence == next_seq:
+                        record.status = "pending"
+                # if there is no approval sequence go directly to pending state
+                elif not record.approve_sequence:
+                    record.status = "pending"
+                if record.status == "pending":
+                    if record.definition_id.notify_on_pending:
+                        record._notify_pending_status(record)


I'm uncertain whether it's advisable to update fields within a computed function if those fields are not explicitly defined as computed (field status).

NOTE: Please let me know your comments regarding my comment on computed fields not being defined as computed explicitly. Could this be causing cache inconsistency?

tbh, I'm a bit overasked by this..
Maybe it is not the ideal situation. (yet, I'm unaware of any alternative method to implement this).
Well, we've deployed this code succesfully in production. W/o any problems.
Judging from the experience it is okay..
But happy to learn from the more experienced dev's.

base_tier_validation/models/tier_validation.py

Co-authored-by: Carlos Lopez <celm1990@hotmail.com>

Dranyel-Bosd

Functional test locally, LGTM 🍾

OCA-git-bot · 2024-01-15T19:00:02Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

celm1990 · 2024-01-15T21:30:35Z

I tested these changes, but I have some doubts. For example, I have three reviews for three specific users:

R1 -> Admin
R2 -> U2
R3 -> U3

Before this commit:
When Admin approved R1, an email is sent to U2 and U3 notifying them that R1 is approved.

After this commit:
When Admin approves R1, an email is sent to both U2 and U3, notifying them that R1 is approved(as previous). Additionally, a new email is sent to U2 and U3 informing them that a new review (R2) has been created. However, for the purpose of this changes, if the intention is to notify only the next user (U2), I hope that only U2 is notified and that no email is sent to U3. This is because if U3 receives an email, they might attempt to review and approve, causing confusion as it is not their turn yet. we would revert back to the initial problem.

Please let me know if I understood the functionality of the module correctly.

bosd · 2024-01-15T22:17:55Z

Additionally, a new email is sent to U2 and U3 informing them that a new review (R2) has been created. However, for the purpose of this changes, if the intention is to notify only the next user (U2), I hope that only U2 is notified and that no email is sent to U3. This is because if U3 receives an email, they might attempt to review and approve, causing confusion as it is not their turn yet. we would revert back to the initial problem.

Yes, I think you understand the functionality correctly.
The main problem before, was that U3 got an invitation email to review.
But he coul'dnt review because it was not his turn yet.
Before this PR, he was unable to get a notification when it actually was his turn.

Another use case is that U3 could be upper management. And only wants to be bothered with messges which made it past the previous tiers.

I'm under the impression you have the functionality and use case correct.
Yet, I'm unable to determine from your feedback if the module passed the functional test.
Dunno if it is the case, but the notification settings might be a bit "misleading"/unclear.
In a sequential flow, I would reccomend to only check the Notify Reviewers on reaching Pending box.
Ignoring all the other notifications, this should happen.

flowchart TD
    A[U0 validation request] -->|Notification message: request to review sent to U1| B[U1 Approves SO]
    B -->|Notification message: request to review sent to  U2| C[U2 approves SO]
    C -->|Notification message: request to review sent to U3| D[U3 approves SO]
    D--> E(The End\n Record = validated)

bosd · 2024-01-15T22:20:59Z

While out of scope of this pr, we might want to look into who is receiving the "approved messages".
To have it go to the original document owner makes sense. In case U3 is upper management, we don't want to bother him with all the back and forth happening in the tiers before.

celm1990

Thanks for clarifying. I unchecked "Notify on Creation" as per your suggestion to reduce emails. However, I noticed that other users are still being notified when reviews are requested. I suspect this is happening because of followers. So, LGTM .

By the way, in the other PR(Improve UI), you can add these fields as optional="show" 😅

NOTE: Please let me know your comments regarding my comment on computed fields not being defined as computed explicitly. Could this be causing cache inconsistency?

bosd · 2024-01-18T18:40:02Z

Oopsie, I answered on the comment above, but it does'nt show up down here.
If more improvements are suggested I'm happy to include them.
But as other mirgrations depend on this base module, ideally we would move forward.

@OCA/tools-maintainers I think this one is ready for merge..

celm1990 · 2024-01-20T15:33:31Z

@LoisRForgeFlow @pedrobaeza, could you take a look at this PR? What do you think, is it possible to merge?

bosd · 2024-01-25T06:27:18Z

@kittiu Can you please review as well?

bosd · 2024-01-25T16:33:03Z

@gurneyalex Can you take a look as well? 🙏

bosd · 2024-02-03T14:33:48Z

@oca/edi-maintainers Can we please fasttrack this one? 🙏

bosd · 2024-02-05T10:27:49Z

Oops, used the wrong handle to fasttrack this one.. @OCA/server-ux-maintainers seems to be the correct one?

Or @rousseldenis Do you have super powers here as well?

rousseldenis · 2024-02-05T10:30:50Z

Oops, used the wrong handle to fasttrack this one.. @OCA/server-ux-maintainers seems to be the correct one?

Or @rousseldenis Do you have super powers here as well?

Nope.

LoisRForgeFlow

Functional review, in general I think this is a great proposal, thanks! Some comments below:

This field seems to be a bit misplaced:

Time to add the sequence handler widget?

There are multiple notifications with the same message. This one is not a blocking comment as I know this was an issue before this change too, but with this sequencial approval it becomes more noisy.

bosd · 2024-02-08T10:14:44Z

Functional review, in general I think this is a great proposal, thanks! Some comments below:

This field seems to be a bit misplaced:

The UI Improvements are made in follow up pr.
#804

Time to add the sequence handler widget?

Great Suggestion..
Will add it, In this PR or the UI PR. In case this one gets merged first.

There are multiple notifications with the same message. This one is not a blocking comment as I know this was an issue before this change too, but with this sequencial approval it becomes more noisy.

I agree, The messages and the content could be improved.
Have been thinking about rewording the message to.. But decided to not do big chances for now.
Adding the reviewer name to which the review is requested from could be usefull.
But will also lead to confusion by request from a group or server action.

@LoisRForgeFlow Are you ok to merge this one as is?
So the other modules which depend on it can be refactored during the migration.

LoisRForgeFlow · 2024-02-08T13:56:22Z

@bosd Yes, I think we can merge as is, and continue the improvements on followup PRs. I don't want to hold this because of my lack of time to review

LoisRForgeFlow · 2024-02-08T13:57:07Z

/ocabot merge minor

OCA-git-bot · 2024-02-08T13:57:16Z

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-800-by-LoisRForgeFlow-bump-minor, awaiting test results.

OCA-git-bot · 2024-02-08T14:00:56Z

Congratulations, your PR was merged at 4600958. Thanks a lot for contributing to OCA. ❤️

[IMP] base_tier_validation: Merge with base_tier_validation_waiting

6232c83

bofiltd approved these changes Jan 10, 2024

View reviewed changes

celm1990 reviewed Jan 11, 2024

View reviewed changes

bosd force-pushed the 17.0-mig-base_tier_validation_merge_waiting branch from c280d4f to 194f9a5 Compare January 11, 2024 06:11

[IMP] base_tier_validation: review comments

395c4a2

Co-authored-by: Carlos Lopez <celm1990@hotmail.com>

bosd force-pushed the 17.0-mig-base_tier_validation_merge_waiting branch from 194f9a5 to 395c4a2 Compare January 11, 2024 06:12

Dranyel-Bosd approved these changes Jan 11, 2024

View reviewed changes

OCA-git-bot added approved ready to merge labels Jan 11, 2024

celm1990 mentioned this pull request Jan 15, 2024

[17.0] base_tier_validation Improve UI #804

Merged

celm1990 approved these changes Jan 15, 2024

View reviewed changes

bosd mentioned this pull request Jan 25, 2024

[17.0][PORT] 699 from 14.0: base_tier_validation multiple comment with approve_sequence_bypass #803

Closed

LoisRForgeFlow reviewed Feb 8, 2024

View reviewed changes

OCA-git-bot added the bot is merging ⏳ label Feb 8, 2024

OCA-git-bot merged commit de8ae97 into OCA:17.0 Feb 8, 2024
5 checks passed

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Feb 8, 2024

LoisRForgeFlow mentioned this pull request Feb 8, 2024

Migration to version 17.0 #752

Open

23 tasks

bosd deleted the 17.0-mig-base_tier_validation_merge_waiting branch February 8, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[17.0][IMP] base_tier_validation: Merge module with base_tier_validation_waiting #800

[17.0][IMP] base_tier_validation: Merge module with base_tier_validation_waiting #800

bosd commented Jan 10, 2024

OCA-git-bot commented Jan 10, 2024

bosd commented Jan 10, 2024

bofiltd left a comment

celm1990 left a comment

celm1990 Jan 11, 2024

bosd Jan 17, 2024

Dranyel-Bosd left a comment

OCA-git-bot commented Jan 15, 2024

celm1990 commented Jan 15, 2024 •

edited

Loading

bosd commented Jan 15, 2024

bosd commented Jan 15, 2024

celm1990 left a comment

bosd commented Jan 18, 2024

celm1990 commented Jan 20, 2024

bosd commented Jan 25, 2024

bosd commented Jan 25, 2024

bosd commented Feb 3, 2024

bosd commented Feb 5, 2024

rousseldenis commented Feb 5, 2024

LoisRForgeFlow left a comment

bosd commented Feb 8, 2024

LoisRForgeFlow commented Feb 8, 2024

LoisRForgeFlow commented Feb 8, 2024

OCA-git-bot commented Feb 8, 2024

OCA-git-bot commented Feb 8, 2024

[17.0][IMP] base_tier_validation: Merge module with base_tier_validation_waiting #800

[17.0][IMP] base_tier_validation: Merge module with base_tier_validation_waiting #800

Conversation

bosd commented Jan 10, 2024

OCA-git-bot commented Jan 10, 2024

bosd commented Jan 10, 2024

bofiltd left a comment

Choose a reason for hiding this comment

celm1990 left a comment

Choose a reason for hiding this comment

celm1990 Jan 11, 2024

Choose a reason for hiding this comment

bosd Jan 17, 2024

Choose a reason for hiding this comment

Dranyel-Bosd left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Jan 15, 2024

celm1990 commented Jan 15, 2024 • edited Loading

bosd commented Jan 15, 2024

bosd commented Jan 15, 2024

celm1990 left a comment

Choose a reason for hiding this comment

bosd commented Jan 18, 2024

celm1990 commented Jan 20, 2024

bosd commented Jan 25, 2024

bosd commented Jan 25, 2024

bosd commented Feb 3, 2024

bosd commented Feb 5, 2024

rousseldenis commented Feb 5, 2024

LoisRForgeFlow left a comment

Choose a reason for hiding this comment

bosd commented Feb 8, 2024

LoisRForgeFlow commented Feb 8, 2024

LoisRForgeFlow commented Feb 8, 2024

OCA-git-bot commented Feb 8, 2024

OCA-git-bot commented Feb 8, 2024

celm1990 commented Jan 15, 2024 •

edited

Loading